cmd/crc/cmd/bundle/download.go (3)

86-96: Consider making the prune retention count configurable.

The keep value is hardcoded to 2. Consider exposing this as a flag (e.g., --keep) to allow users to control how many bundles to retain.

 func getPruneCmd() *cobra.Command {
-	return &cobra.Command{
+	cmd := &cobra.Command{
 		Use:   "prune",
 		Short: "Prune old CRC bundles",
 		Long:  "Keep only the most recent bundles and delete older ones to save space.",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Default keep 2 most recent
-			return runPrune(2)
+			keep, _ := cmd.Flags().GetInt("keep")
+			return runPrune(keep)
 		},
 	}
+	cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep")
+	return cmd
 }

---

253-261: Consider failing early when version resolution fails for partial versions.

When a partial version (e.g., 4.19) cannot be resolved, the code warns and continues with the original version string. This will likely result in a 404 error later since 4.19 isn't a valid bundle path. Consider failing early for better UX.

 	resolvedVersion, err := resolveOpenShiftVersion(preset, version, verbose)
 	if err != nil {
-		logging.Warnf("Could not resolve version %s: %v. Trying with original version string.", version, err)
-	} else if resolvedVersion != version {
+		// Only warn if it looks like a full version that just wasn't found
+		partialVersionRegex := regexp.MustCompile(`^(\d+\.\d+)$`)
+		if partialVersionRegex.MatchString(version) {
+			return fmt.Errorf("could not resolve version %s: %w", version, err)
+		}
+		logging.Warnf("Could not resolve version %s: %v. Trying with original version string.", version, err)
+	}
+	if resolvedVersion != "" && resolvedVersion != version {
 		if verbose {
 			logging.Infof("Resolved version %s to %s", version, resolvedVersion)
 		}
 		version = resolvedVersion
 	}

---

370-378: Consider adding context support for request cancellation.

The HTTP request doesn't use a context, which means long-running requests can't be cancelled by the user (e.g., via Ctrl+C). Consider accepting a context parameter.

-func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) {
+func fetchAvailableVersions(ctx context.Context, preset crcPreset.Preset) ([]*semver.Version, error) {
 	baseURL := fmt.Sprintf("https://mirror.openshift.com/pub/openshift-v4/clients/crc/bundles/%s/", preset.String())
 
 	client := &http.Client{
 		Transport: httpproxy.HTTPTransport(),
 		Timeout:   10 * time.Second,
 	}
 
-	req, err := http.NewRequest("GET", baseURL, nil)
+	req, err := http.NewRequestWithContext(ctx, "GET", baseURL, nil)
 	if err != nil {
 		return nil, err
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/download.go (1)

154-159: Potential nil pointer dereference when Info() fails.

If Info() returns an error (e.g., file deleted between ReadDir and Info call), the returned FileInfo is nil, causing a panic when accessing ModTime().

Apply this diff to handle the error case:

 	sort.Slice(bundleFiles, func(i, j int) bool {
-		infoI, _ := bundleFiles[i].Info()
-		infoJ, _ := bundleFiles[j].Info()
+		infoI, errI := bundleFiles[i].Info()
+		infoJ, errJ := bundleFiles[j].Info()
+		if errI != nil || errJ != nil {
+			return errJ != nil && errI == nil
+		}
 		return infoI.ModTime().After(infoJ.ModTime())
 	})

cmd/crc/cmd/bundle/download.go (3)

86-96: Consider making the keep count configurable.

The hardcoded keep=2 value could be exposed as a flag for flexibility (e.g., --keep=N).

 func getPruneCmd() *cobra.Command {
-	return &cobra.Command{
+	cmd := &cobra.Command{
 		Use:   "prune",
 		Short: "Prune old CRC bundles",
 		Long:  "Keep only the most recent bundles and delete older ones to save space.",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Default keep 2 most recent
-			return runPrune(2)
+			keep, _ := cmd.Flags().GetInt("keep")
+			return runPrune(keep)
 		},
 	}
+	cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep")
+	return cmd
 }

---

115-117: Prefer os.Remove for regular files.

os.RemoveAll is designed for recursive directory removal. For single files, os.Remove is more explicit and semantically correct.

-			if err := os.RemoveAll(filePath); err != nil {
+			if err := os.Remove(filePath); err != nil {

---

365-379: Consider propagating context for cancellation support.

The HTTP request doesn't use context, preventing graceful cancellation (e.g., via Ctrl+C). For CLI tools this is usually acceptable, but adding context would improve the user experience.

-func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) {
+func fetchAvailableVersions(ctx context.Context, preset crcPreset.Preset) ([]*semver.Version, error) {
 	baseURL := fmt.Sprintf("https://mirror.openshift.com/pub/openshift-v4/clients/crc/bundles/%s/", preset.String())
 
 	client := &http.Client{
 		Transport: httpproxy.HTTPTransport(),
 		Timeout:   10 * time.Second,
 	}
 
-	req, err := http.NewRequest("GET", baseURL, nil)
+	req, err := http.NewRequestWithContext(ctx, "GET", baseURL, nil)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/download.go (1)

152-157: Potential nil pointer dereference when Info() fails.

This issue was flagged in a previous review. If Info() returns an error, the returned FileInfo is nil, causing a panic when accessing ModTime().

 	// Sort by modification time, newest first
 	sort.Slice(bundleFiles, func(i, j int) bool {
-		infoI, _ := bundleFiles[i].Info()
-		infoJ, _ := bundleFiles[j].Info()
+		infoI, errI := bundleFiles[i].Info()
+		infoJ, errJ := bundleFiles[j].Info()
+		if errI != nil || errJ != nil {
+			// If we can't get info, treat as oldest (sort to end for pruning)
+			return errJ != nil && errI == nil
+		}
 		return infoI.ModTime().After(infoJ.ModTime())
 	})

cmd/crc/cmd/bundle/download.go (4)

84-94: Consider making the keep count configurable.

The hardcoded keep=2 value works, but users might want to retain more or fewer bundles depending on their use case.

 func getPruneCmd() *cobra.Command {
-	return &cobra.Command{
+	cmd := &cobra.Command{
 		Use:   "prune",
 		Short: "Prune old CRC bundles",
 		Long:  "Keep only the most recent bundles and delete older ones to save space.",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Default keep 2 most recent
-			return runPrune(2)
+			keep, _ := cmd.Flags().GetInt("keep")
+			return runPrune(keep)
 		},
 	}
+	cmd.Flags().IntP("keep", "k", 2, "Number of recent bundles to keep")
+	return cmd
 }

---

214-221: Cache check is not architecture-aware.

This function uses BundleForPreset which hardcodes runtime.GOARCH. Bundles downloaded with an explicit architecture argument won't show as cached in the list output. Consider adding an optional arch parameter or using BundleName directly.

-func isBundleCached(preset crcPreset.Preset, version string) bool {
-	bundleName := constants.BundleForPreset(preset, version)
+func isBundleCached(preset crcPreset.Preset, version string, arch string) bool {
+	bundleName := constants.BundleName(preset, version, arch)
 	bundlePath := filepath.Join(constants.MachineCacheDir, bundleName)
 	if _, err := os.Stat(bundlePath); err == nil {
 		return true
 	}
 	return false
 }

---

319-327: Consider more robust hash file parsing.

The current parsing assumes exact format "hash filename". Using strings.Fields would be more resilient to whitespace variations.

 	lines := strings.Split(verifiedHashes, "\n")
 	for _, line := range lines {
-		if strings.HasSuffix(line, bundleName) {
-			sha256sum := strings.TrimSuffix(line, "  "+bundleName)
-			return sha256sum, nil
+		fields := strings.Fields(line)
+		if len(fields) == 2 && fields[1] == bundleName {
+			return fields[0], nil
 		}
 	}

---

333-346: Consider using context for request cancellation.

The HTTP client has a timeout, but using http.NewRequestWithContext would allow callers to cancel long-running requests and integrate better with the rest of the codebase.

-func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) {
+func fetchAvailableVersions(ctx context.Context, preset crcPreset.Preset) ([]*semver.Version, error) {
 	// Base URL for the preset
 	baseURL := fmt.Sprintf("https://mirror.openshift.com/pub/openshift-v4/clients/crc/bundles/%s/", preset.String())
 
 	client := &http.Client{
 		Transport: httpproxy.HTTPTransport(),
 		Timeout:   10 * time.Second,
 	}
 
-	req, err := http.NewRequest("GET", baseURL, nil)
+	req, err := http.NewRequestWithContext(ctx, "GET", baseURL, nil)
 	if err != nil {
 		return nil, err
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/download.go (2)

96-126: runClear swallows deletion failures but still reports full success

If any os.RemoveAll call fails, the error is only logged and runClear still returns nil, potentially printing “Cleared cached bundles” even when some bundles remain. This can mislead users and makes troubleshooting harder. (This was already raised in a previous review.)

You can track whether any deletions failed and return a non‑nil error when they do, while keeping per‑file logging. For example:

 func runClear() error {
@@
-	cleared := false
+	cleared := false
+	var lastErr error
 	for _, file := range files {
 		if strings.HasSuffix(file.Name(), ".crcbundle") {
 			filePath := filepath.Join(cacheDir, file.Name())
 			logging.Infof("Deleting %s", filePath)
 			if err := os.RemoveAll(filePath); err != nil {
 				logging.Errorf("Failed to remove %s: %v", filePath, err)
+				lastErr = err
+				continue
 			}
 			cleared = true
 		}
 	}
@@
 	} else {
 		logging.Infof("Cleared cached bundles in %s", cacheDir)
 	}
-	return nil
+	return lastErr
 }

---

128-169: Fix potential panic in runPrune sort comparator and surface deletion failures

The sort comparator ignores errors from DirEntry.Info() and then calls ModTime() unconditionally. If Info() fails, FileInfo can be nil, causing a panic. This was already pointed out earlier and is still present. Additionally, like runClear, deletion failures are only logged and runPrune still returns nil.

You can make the sort and deletion logic safer, for example:

 	// Sort by modification time, newest first
 	sort.Slice(bundleFiles, func(i, j int) bool {
-		infoI, _ := bundleFiles[i].Info()
-		infoJ, _ := bundleFiles[j].Info()
-		return infoI.ModTime().After(infoJ.ModTime())
+		infoI, errI := bundleFiles[i].Info()
+		infoJ, errJ := bundleFiles[j].Info()
+
+		switch {
+		case errI != nil && errJ != nil:
+			// Fallback to name ordering if both fail
+			return bundleFiles[i].Name() < bundleFiles[j].Name()
+		case errI != nil:
+			// Treat missing info as oldest
+			return false
+		case errJ != nil:
+			return true
+		default:
+			return infoI.ModTime().After(infoJ.ModTime())
+		}
 	})
@@
-	for i := keep; i < len(bundleFiles); i++ {
+	var lastErr error
+	for i := keep; i < len(bundleFiles); i++ {
 		file := bundleFiles[i]
 		filePath := filepath.Join(cacheDir, file.Name())
 		logging.Infof("Pruning old bundle: %s", file.Name())
 		if err := os.RemoveAll(filePath); err != nil {
 			logging.Errorf("Failed to remove %s: %v", filePath, err)
+			lastErr = err
 		}
 	}
 
-	return nil
+	return lastErr

pkg/crc/constants/constants.go (1)

78-105: BundleName helper is a good extraction; OS is still hard-wired to runtime.GOOS

The refactor keeps existing behavior for BundleForPreset while enabling explicit arch selection via BundleName, which is what the new download workflow needs. If you ever need to construct bundle names for a non-local OS (e.g., generating docs or tooling for another platform), consider adding an OS parameter instead of relying on runtime.GOOS, but that’s not required for this PR.

cmd/crc/cmd/bundle/download.go (4)

29-94: Command wiring and subcommands look cohesive

The download command structure with list, clear, and prune subcommands is clear and consistent with the rest of the CLI. Hard‑wiring prune to keep 2 bundles is fine for now, but if you expect different retention policies, consider a --keep flag (defaulting to 2) to make this configurable without code changes.

---

171-221: list only marks bundles cached for the default arch; multi‑arch caches aren’t reflected

runList uses isBundleCached(preset, v.String()), and isBundleCached delegates to constants.BundleForPreset, which always uses runtime.GOARCH. If a user downloads a different architecture (e.g., arm64 on an amd64 host), that version will never be shown as “(cached)” even though a bundle file exists.

If multi‑arch usage is a target for this command, consider either:

• Scanning the cache directory for any file matching the version regardless of arch (e.g., crc_*_<version>_*.crcbundle) and marking the version as cached if any match exists, or
• Extending list to show per‑arch information explicitly.

As a smaller UX tweak, you might also want to warn when a provided list filter argument cannot be parsed as either Major.Minor or Major.Minor.Patch, instead of silently listing everything.

---

223-297: Download flow and version resolution look solid; minor logging nit

The control flow for default vs custom version, version resolution, and signature verification is coherent and aligns with the new BundleName helper. One small clarity improvement: the “Destination” debug log currently prints constants.MachineCacheDir, while the actual target is bundlePath / defaultBundlePath. Logging the full file path would be more precise when debugging user reports.

-	logging.Debugf("Destination: %s", constants.MachineCacheDir)
+	logging.Debugf("Destination: %s", bundlePath)

(and similarly for the default‑bundle branch).

---

299-327: Hash-line parsing assumes a very specific sha256sum format

getVerifiedHashForCustomVersion trims " "+bundleName from the end of the line to extract the hash. This works as long as the file uses exactly two spaces before the filename and no extra markers (like *), but it’s a bit brittle: any formatting change in sha256sum.txt will cause a failure to find the hash, even though the signature verifies.

You could make this more robust by splitting on whitespace and matching the filename field, e.g.:

 for _, line := range lines {
-	if strings.HasSuffix(line, bundleName) {
-		sha256sum := strings.TrimSuffix(line, "  "+bundleName)
-		return sha256sum, nil
-	}
+	fields := strings.Fields(line)
+	if len(fields) >= 2 && fields[len(fields)-1] == bundleName {
+		return fields[0], nil
+	}
 }

This keeps behavior the same but tolerates minor formatting changes in the checksum file.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/clear.go (1)

36-46: Partial deletion failures are silently swallowed and incorrectly counted as cleared.

The cleared flag is set to true even when os.RemoveAll fails. This means:

1. The function returns nil even when some deletions failed
2. The success message is shown despite failures

 	cleared := false
+	var lastErr error
 	for _, file := range files {
 		if strings.HasSuffix(file.Name(), ".crcbundle") {
 			filePath := filepath.Join(cacheDir, file.Name())
 			logging.Infof("Deleting %s", filePath)
 			if err := os.RemoveAll(filePath); err != nil {
 				logging.Errorf("Failed to remove %s: %v", filePath, err)
+				lastErr = err
+			} else {
+				cleared = true
 			}
-			cleared = true
 		}
 	}
 
 	if !cleared {
 		logging.Infof("No bundles found in %s", cacheDir)
 	} else {
 		logging.Infof("Cleared cached bundles in %s", cacheDir)
 	}
-	return nil
+	return lastErr

cmd/crc/cmd/bundle/prune.go (1)

50-55: Potential nil pointer dereference when Info() fails.

If Info() returns an error (e.g., file deleted between ReadDir and Info call, or permission issue), the returned FileInfo is nil, causing a panic when accessing ModTime().

 	// Sort by modification time, newest first
 	sort.Slice(bundleFiles, func(i, j int) bool {
-		infoI, _ := bundleFiles[i].Info()
-		infoJ, _ := bundleFiles[j].Info()
+		infoI, errI := bundleFiles[i].Info()
+		infoJ, errJ := bundleFiles[j].Info()
+		if errI != nil || errJ != nil {
+			// If we can't get info, treat as oldest (sort to end for pruning)
+			return errJ != nil && errI == nil
+		}
 		return infoI.ModTime().After(infoJ.ModTime())
 	})

cmd/crc/cmd/bundle/list.go (1)

36-46: Consider informing the user when filter parsing fails.

When an argument is provided but cannot be parsed as a valid version (neither as partial nor full), the filter remains nil and all versions are displayed without any indication to the user that their filter was ignored.

 	var filter *semver.Version
 	if len(args) > 0 {
 		v, err := semver.NewVersion(args[0] + ".0") // Treat 4.19 as 4.19.0 for partial matching
 		if err == nil {
 			filter = v
 		} else {
 			// Try parsing as full version just in case
 			v, err = semver.NewVersion(args[0])
 			if err == nil {
 				filter = v
+			} else {
+				logging.Warnf("Invalid version filter %q, showing all versions", args[0])
 			}
 		}
 	}

cmd/crc/cmd/bundle/prune.go (1)

14-24: Consider making keep configurable via a flag.

The keep value is hardcoded to 2. Users may want to retain a different number of bundles.

 func getPruneCmd() *cobra.Command {
-	return &cobra.Command{
+	cmd := &cobra.Command{
 		Use:   "prune",
 		Short: "Prune old CRC bundles",
 		Long:  "Keep only the most recent bundles and delete older ones to save space.",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Default keep 2 most recent
-			return runPrune(2)
+			keep, _ := cmd.Flags().GetInt("keep")
+			return runPrune(keep)
 		},
 	}
+	cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep")
+	return cmd
 }

cmd/crc/cmd/bundle/download.go (1)

148-154: Hash parsing may fail silently if format differs.

The hash extraction assumes the sha256sum format with exactly two spaces between hash and filename ("hash filename"). If the format varies (e.g., single space, or * prefix for binary mode), the parsing will fail to match or extract incorrectly.

 	lines := strings.Split(verifiedHashes, "\n")
 	for _, line := range lines {
-		if strings.HasSuffix(line, bundleName) {
-			sha256sum := strings.TrimSuffix(line, "  "+bundleName)
-			return sha256sum, nil
+		// sha256sum format: "hash  filename" or "hash *filename" (binary mode)
+		line = strings.TrimSpace(line)
+		if strings.HasSuffix(line, bundleName) || strings.HasSuffix(line, "*"+bundleName) {
+			fields := strings.Fields(line)
+			if len(fields) >= 2 {
+				return fields[0], nil
+			}
 		}
 	}

cmd/crc/cmd/bundle/util.go (3)

26-26: Consider extracting regex patterns and making timeout configurable.

Optional improvements for maintainability:

1. Extract the regex patterns (lines 50, 73) as package-level constants for easier testing and maintenance.
2. Consider making the 10-second timeout configurable via a constant or parameter for users with slow network connections.

Example refactor:

+const (
+	mirrorFetchTimeout = 10 * time.Second
+	versionHrefPattern = `href=["']?\.?/?(\d+\.\d+\.\d+)/?["']?`
+	versionSimplePattern = `>(\d+\.\d+\.\d+)/?<`
+)
+
 func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) {
 	// Base URL for the preset
 	baseURL := fmt.Sprintf("%s/%s/", constants.DefaultMirrorURL, preset.String())
 
 	client := &http.Client{
 		Transport: httpproxy.HTTPTransport(),
-		Timeout:   10 * time.Second,
+		Timeout:   mirrorFetchTimeout,
 	}
 
 	// ... request handling ...
 
 	// Parse the HTML directory listing to find version directories
-	versionRegex := regexp.MustCompile(`href=["']?\.?/?(\d+\.\d+\.\d+)/?["']?`)
+	versionRegex := regexp.MustCompile(versionHrefPattern)
 
 	// ... matching logic ...
 
 	// If regex failed, try a simpler one for directory names in text
 	if len(versions) == 0 {
-		simpleRegex := regexp.MustCompile(`>(\d+\.\d+\.\d+)/?<`)
+		simpleRegex := regexp.MustCompile(versionSimplePattern)

Also applies to: 50-50, 73-73

---

103-106: Non-standard version formats are passed through without validation.

When the input doesn't match Major.Minor.Patch or Major.Minor patterns, the function returns it as-is (line 105). This could lead to errors downstream if users provide malformed versions like "4.x", "latest", or "4.19-rc1".

Consider validating these cases or documenting that arbitrary strings are accepted (e.g., for tags).

Apply this diff to add validation for common edge cases:

 	// If not Major.Minor, return as is (could be a tag or other format user intends)
 	partialVersionRegex := regexp.MustCompile(`^(\d+\.\d+)$`)
 	if !partialVersionRegex.MatchString(inputVersion) {
-		return inputVersion, nil
+		// Check if it's at least a valid-looking tag/version string
+		if matched, _ := regexp.MatchString(`^[\w][\w.-]*$`, inputVersion); !matched {
+			return "", fmt.Errorf("invalid version format: %s", inputVersion)
+		}
+		logging.Debugf("Using version string as-is: %s", inputVersion)
+		return inputVersion, nil
 	}

Alternatively, document in a comment that arbitrary version strings are accepted for flexibility with tags.

---

129-136: Improve error handling to distinguish between "not cached" and other errors.

The function returns false for any os.Stat error, which masks issues like permission denied or corrupted filesystem paths. This could make debugging difficult if cache directory permissions are incorrect.

Apply this diff to be more explicit about error types:

+	"errors"
+
 func isBundleCached(preset crcPreset.Preset, version string) bool {
 	bundleName := constants.BundleForPreset(preset, version)
 	bundlePath := filepath.Join(constants.MachineCacheDir, bundleName)
-	if _, err := os.Stat(bundlePath); err == nil {
-		return true
+	_, err := os.Stat(bundlePath)
+	if err == nil {
+		return true
+	}
+	if errors.Is(err, os.ErrNotExist) {
+		return false
 	}
+	// Log unexpected errors but treat as not cached
+	logging.Debugf("Error checking bundle cache: %v", err)
 	return false
 }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/root_test.go (1)

26-29: Critical: Manpage test expectations still don't match the command structure.

Despite the past review comment being marked as addressed, the issue persists. These manpage names imply clear, list, and prune are subcommands of download (e.g., crc bundle download clear). However, in bundle.go (lines 18-21), these commands are added as direct subcommands of bundle via bundleCmd.AddCommand(), making them peers of download rather than its children.

The correct manpage names should be:

• crc-bundle-clear.1 (not crc-bundle-download-clear.1)
• crc-bundle-list.1 (not crc-bundle-download-list.1)
• crc-bundle-prune.1 (not crc-bundle-download-prune.1)
• crc-bundle-download.1 ✓ (this one is correct)

🔎 Apply this diff to fix the manpage expectations:

 	assert.ElementsMatch(t, []string{
-		"crc-bundle-download-clear.1",
-		"crc-bundle-download-list.1",
-		"crc-bundle-download-prune.1",
+		"crc-bundle-clear.1",
+		"crc-bundle-list.1",
+		"crc-bundle-prune.1",
 		"crc-bundle-download.1",

cmd/crc/cmd/bundle/util.go (2)

24-27: Consider increasing the HTTP timeout for slower connections.

The 10-second timeout might be insufficient for users on slow or unstable network connections, potentially causing false negatives when fetching version listings from the mirror.

🔎 Consider increasing the timeout:

 	client := &http.Client{
 		Transport: httpproxy.HTTPTransport(),
-		Timeout:   10 * time.Second,
+		Timeout:   30 * time.Second,
 	}

---

49-88: Replace fragile regex HTML parsing with a proper HTML parser.

The code parses HTML directory listings from mirror.openshift.com using regex patterns, which is fragile and vulnerable to format changes. Mirror.openshift.com provides directory listings in HTML format, not a structured API. While a fallback regex is provided, both patterns could fail if the HTML structure changes.

Consider using Goquery, a popular HTML parsing library for Go with jQuery-like syntax that performs HTML document traversals and selections efficiently using CSS selectors, or Go's standard net/html package for parsing and manipulating HTML documents in a structured manner.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/prune.go (1)

14-23: Consider exposing keep as a CLI flag.

The keep parameter is hardcoded to 2. For flexibility, consider adding a --keep flag so users can specify how many bundles to retain.

🔎 Example implementation

 func getPruneCmd() *cobra.Command {
-	return &cobra.Command{
+	cmd := &cobra.Command{
 		Use:   "prune",
 		Short: "Prune old CRC bundles",
 		Long:  "Keep only the most recent bundles and delete older ones to save space.",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Default keep 2 most recent
-			return runPrune(2)
+			keep, _ := cmd.Flags().GetInt("keep")
+			return runPrune(keep)
 		},
 	}
+	cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep")
+	return cmd
 }

cmd/crc/cmd/bundle/download.go (2)

28-30: Ignoring flag retrieval errors may hide configuration issues.

The errors from GetBool and GetString are discarded. While these typically don't fail for registered flags, silently ignoring errors can hide misconfigurations during development or refactoring.

🔎 Consider logging or handling errors

 		RunE: func(cmd *cobra.Command, args []string) error {
-			force, _ := cmd.Flags().GetBool("force")
-			presetStr, _ := cmd.Flags().GetString("preset")
+			force, err := cmd.Flags().GetBool("force")
+			if err != nil {
+				return fmt.Errorf("failed to get force flag: %w", err)
+			}
+			presetStr, err := cmd.Flags().GetString("preset")
+			if err != nil {
+				return fmt.Errorf("failed to get preset flag: %w", err)
+			}

---

86-89: Consider validating the architecture argument.

The architecture value from args[1] is used directly without validation. While an invalid value will result in a download failure (404), validating against known architectures (e.g., amd64, arm64) would provide a clearer error message.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/util.go (2)

20-93: Robust implementation with good fallback logic.

The function properly handles proxy configuration, timeout, error cases, and has a fallback regex when the primary pattern fails. The deduplication and sorting logic is correct.

Consider adding a debug log when falling back to the simpler regex to aid troubleshooting:

🔎 Optional enhancement

 	// If regex failed, try a simpler one for directory names in text
 	if len(versions) == 0 {
+		logging.Debugf("Primary regex found no versions, trying fallback pattern")
 		simpleRegex := regexp.MustCompile(`>(\d+\.\d+\.\d+)/?<`)

---

95-127: Function name is misleading.

The function is named resolveOpenShiftVersion but it works for all presets (OpenShift, OKD, Microshift), not just OpenShift. Consider renaming to resolveVersion or resolveBundleVersion for clarity.

🔎 Suggested rename

-func resolveOpenShiftVersion(preset crcPreset.Preset, inputVersion string) (string, error) {
+func resolveVersion(preset crcPreset.Preset, inputVersion string) (string, error) {

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

cmd/crc/cmd/bundle/download.go (1)

148-155: Hash extraction could be more robust.

The current parsing assumes the exact format hash filename with two spaces. Consider using strings.Fields to split on whitespace, which handles varying whitespace and validates that exactly two fields exist.

🔎 Proposed fix

 	lines := strings.Split(verifiedHashes, "\n")
 	for _, line := range lines {
-		if strings.HasSuffix(line, bundleName) {
-			sha256sum := strings.TrimSuffix(line, "  "+bundleName)
-			return sha256sum, nil
+		fields := strings.Fields(line)
+		if len(fields) == 2 && fields[1] == bundleName {
+			return fields[0], nil
 		}
 	}

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro